Reviewed 6 of 6 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @mojganii)

ios/MullvadVPN/View controllers/ProblemReport/ProblemReportInteractor.swift line 29 at r1 (raw file):

    func loadLogFiles(completion: @escaping () -> Void) {
        DispatchQueue

Is this queueu necessary when consolidatedLog.addLogFiles already runs in a concurrent queue? Same for the calls to consolidatedLog.string below.

ios/MullvadVPN/Classes/ConsolidatedApplicationLog.swift line 55 at r1 (raw file):

    func addLogFiles(fileURLs: [URL], completion: @escaping () -> Void = {}) {
        logQueue.async(flags: .barrier) {

Why do we use .barrier?

ios/MullvadVPN/Classes/ConsolidatedApplicationLog.swift line 113 at r1 (raw file):

        let redactedPath = redact(string: path)

        if let lossyString = readFileLossy(path: path, maxBytes: bufferSize) {

Why getting rid of the gobal var? .logMaximumFileSize ensures all log related activities use the same size.

Reviewed 2 of 6 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @mojganii)

ios/MullvadVPNTests/MullvadVPN/Log/ConsolidatedApplicationLogTests.swift line 28 at r2 (raw file):

    override func tearDownWithError() throws {
        super.tearDown()

We should call the correct super function, tearDownWithError here

ios/MullvadVPNTests/MullvadVPN/Log/ConsolidatedApplicationLogTests.swift line 39 at r2 (raw file):

    func testAddLogFiles() {
        var string = ""
        let expectation = self.expectation(description: "Log files added")

nit
no need for self here and below

ios/MullvadVPNTests/MullvadVPN/Log/ConsolidatedApplicationLogTests.swift line 80 at r2 (raw file):

    }

    func testAddLogFilesRaceCondition() {

We discussed this offline, and decided to remove this test and testReportedStringRaceCondition as it doesn't really test against race conditions.

ios/MullvadVPN/Classes/ConsolidatedApplicationLog.swift line 133 at r2 (raw file):

            String(decoding: data, as: UTF8.self)
                .drop { ch in
                    // Drop leading replacement characters produced when decoding data

That comment was useful, we should leave it

ios/MullvadVPN/View controllers/ProblemReport/ProblemReportInteractor.swift line 30 at r2 (raw file):

    func loadLogFiles(completion: @escaping () -> Void) {
        DispatchQueue
            .global()

We don't need to dispatch on the global queue here since we're already doing this in a private concurrent queue.
The same applies to all uses of global in this file.

If we absolutely need to dispatch off the UI thread here, the ProblemReportInteractor class can use a private dispatch queue.

Reviewable status: 2 of 7 files reviewed, 11 unresolved discussions (waiting on @buggmagnet and @rablador)

ios/MullvadVPN/Classes/ConsolidatedApplicationLog.swift line 55 at r1 (raw file):

Since the queue in concurrent, we need to be sure manipulation is executing in thread safe manner.

ios/MullvadVPN/Classes/ConsolidatedApplicationLog.swift line 113 at r1 (raw file):

I'll add it through dependency injection.

ios/MullvadVPN/Classes/ConsolidatedApplicationLog.swift line 133 at r2 (raw file):

It was unintentional.

ios/MullvadVPN/View controllers/ProblemReport/ProblemReportInteractor.swift line 30 at r2 (raw file):

We don't need that here. I made mistake.

ios/MullvadVPNTests/MullvadVPN/Log/ConsolidatedApplicationLogTests.swift line 28 at r2 (raw file):

good catch.

ios/MullvadVPNTests/MullvadVPN/Log/ConsolidatedApplicationLogTests.swift line 39 at r2 (raw file):

Nice.

ios/MullvadVPNTests/MullvadVPN/Log/ConsolidatedApplicationLogTests.swift line 80 at r2 (raw file):

Done.

ios/MullvadVPN/Classes/ConsolidatedApplicationLog.swift line 36 at r3 (raw file):

    private let logQueue = DispatchQueue(
        label: "com.mullvad.consolidation.logs.queue",
        attributes: .concurrent

since there is I/O Operations and they can sometimes cause heavy process, I rather to user serial queue instead of concurrent one.

ios/MullvadVPN/Classes/ConsolidatedApplicationLog.swift line 68 at r4 (raw file):

        logQueue.async(flags: .barrier) {
            self.logs.append(LogAttachment(label: message, content: redactedError))
            DispatchQueue.main.async {

switch to main for calling the completion, since ConsolidatedApplicationLog belongs and interacts with the main app.

ios/MullvadVPN/Classes/ConsolidatedApplicationLog.swift line 76 at r4 (raw file):

    var string: String {
        var result = ""
        logQueue.sync {

Since It's a serial queue anymore then for avoiding deadlock, I removed invoking write function an the sync process.

Reviewed 5 of 5 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @buggmagnet and @mojganii)

ios/MullvadVPNTests/MullvadVPN/Log/ConsolidatedApplicationLogTests.swift line 23 at r3 (raw file):

            redactCustomStrings: mockRedactStrings,
            redactContainerPathsForSecurityGroupIdentifiers: mockSecurityGroupIdentifiers,
            bufferSize: 65_536

How come we use a different buffer size than the default in prod code?

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @buggmagnet and @rablador)

ios/MullvadVPNTests/MullvadVPN/Log/ConsolidatedApplicationLogTests.swift line 23 at r3 (raw file):

since it's a test and the content is short, then I think it doesn't matter not to use that buffer size on prod

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @buggmagnet)

Reviewed all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @mojganii)

ios/MullvadVPN/Classes/ConsolidatedApplicationLog.swift line 157 at r4 (raw file):

        }

        let data = fileHandle.readData(ofLength: Int(bufferSize))

This method seems deprecated, we should use its replacement

public func read(upToCount count: Int) throws -> Data?

ios/MullvadVPN/Classes/ConsolidatedApplicationLog.swift line 160 at r4 (raw file):

        let replacementCharacter = Character(UTF8.decode(UTF8.encodedReplacementCharacter))
        let lossyString = String(
            String(decoding: data, as: UTF8.self)

👮‍♀️ ⚠️ Optional Data -> String Conversion Violation: Prefer failable String(data:encoding:)initializer when convertingDatatoString (optional_data_string_conversion)

ios/MullvadVPNTests/MullvadVPN/Log/ConsolidatedApplicationLogTests.swift line 38 at r4 (raw file):

    }

    func testAddLogFiles() {

Somehow, this test is broken now

Reviewed 5 of 5 files at r5, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @mojganii)

ios/MullvadVPN/Classes/ConsolidatedApplicationLog.swift line 77 at r5 (raw file):

        var result = ""
        logQueue.sync {
            if !logs.isEmpty {

This seems to be almost identical to write() below. Can we create a new function for the constructiom of result/localOutput?

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @buggmagnet and @rablador)

ios/MullvadVPN/Classes/ConsolidatedApplicationLog.swift line 157 at r4 (raw file):

Done.

ios/MullvadVPN/Classes/ConsolidatedApplicationLog.swift line 160 at r4 (raw file):

Done.

ios/MullvadVPN/Classes/ConsolidatedApplicationLog.swift line 77 at r5 (raw file):

No, that can cause deadlock.

ios/MullvadVPNTests/MullvadVPN/Log/ConsolidatedApplicationLogTests.swift line 38 at r4 (raw file):

Fixed.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @mojganii and @rablador)

ios/MullvadVPN/Classes/ConsolidatedApplicationLog.swift line 77 at r5 (raw file):

I agree with @rablador, we can create such a function

Note that you can also use dispatchPrecondition(condition: .onQueue(logQueue)) (or the .notOnQueue variant) to make assertions about deadlocks and whatnot.

    func formatHeader() -> String {
        var result = ""
        result += "System information:\n"
        for (key, value) in metadata {
            result += "\(key.rawValue): \(value)\n"
        }
        result += "\n"
        for attachment in logs {
            result += "\(kLogDelimiter)\n"
            result += "\(attachment.label)\n"
            result += "\(kLogDelimiter)\n"
            result += "\(attachment.content)\n\n"
        }

        return result
    }

    var string: String {
        var result = ""
        logQueue.sync {
            if !logs.isEmpty {
                result = formatHeader()
            }
        }
        return result
    }

    func write(to stream: inout some TextOutputStream) {
        logQueue.sync {
            var localOutput = ""
            if !logs.isEmpty {
                localOutput = formatHeader()
                stream.write(localOutput)
            }
        }
    }

ios/MullvadSettings/SettingsManager.swift line 50 at r5 (raw file):

    public static func getLastUsedAccount() throws -> String {
        let data = try store.read(key: .lastUsedAccount)
        return String(bytes: data, encoding: .utf8) ?? ""

We should throw a StringDecodingError instead of silently failing and returning an empty string here.

ios/MullvadVPN/View controllers/ProblemReport/ProblemReportInteractor.swift line 24 at r5 (raw file):

        self.tunnelManager = tunnelManager
        self.consolidatedLog = ConsolidatedApplicationLog(
            redactCustomStrings: [tunnelManager.deviceState.accountData?.number].compactMap { $0 },

I realize that this redactCustomStrings is not a good design.
It only does something if the user is logged in when sending logs, otherwise this will be empty (because no accountData.number when logged out) and won't redact anything.

Thankfully, we have a redactAccountNumber function that uses a regex to redact account numbers, otherwise we'd have a pretty major problem.

We should delete this false sense of privacy, and make sure the regex we use (for now) is solid instead.

Reviewable status: 7 of 9 files reviewed, 3 unresolved discussions (waiting on @buggmagnet and @rablador)

ios/MullvadSettings/SettingsManager.swift line 50 at r5 (raw file):

but the function String(bytes: data, encoding: .utf8) doesn't throw any thing and the new one is the same as what they were.

ios/MullvadVPN/Classes/ConsolidatedApplicationLog.swift line 77 at r5 (raw file):

dispatchPrecondition(condition: .onQueue(logQueue)) can't prevent from probably deadlock, it's just for checking, I found another way to do that.

ios/MullvadVPN/View controllers/ProblemReport/ProblemReportInteractor.swift line 24 at r5 (raw file):

I know it's good to improve code when there is room to do that but It's beyond the current issue I opened PR for. since it's adding through DI then I changed that to accept nil and mange that in ConsolidatedApplicationLog

Reviewed 2 of 2 files at r6, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @buggmagnet)

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mojganii)

ios/MullvadSettings/SettingsManager.swift line 50 at r5 (raw file):

That init can return a nil value, and this API is already declared as throws so the call site expects to handle errors.
In general, it's bad practice to hide errors like this, because it makes it significantly harder to debug when things go wrong.

ios/MullvadVPN/Classes/ConsolidatedApplicationLog.swift line 77 at r5 (raw file):

You're right that dispatchPrecondition does not prevent deadlocks, but that isn't the point of using that API.
The point is that whenever you cannot guarantee that a call happens on a specific dispatch queue, this will blow up and give you a stack trace, which will lead to the root cause.

Reviewable status: 5 of 10 files reviewed, 2 unresolved discussions (waiting on @buggmagnet and @rablador)

ios/MullvadSettings/SettingsManager.swift line 50 at r5 (raw file):

I agree to throw exception when it's not able to cast data to intended encoding but I'm just telling it was the thing we had before :)

ios/MullvadVPN/Classes/ConsolidatedApplicationLog.swift line 77 at r5 (raw file):

I agree but with the current implementation I suppose we don't need that anymore.

Reviewed 5 of 5 files at r7, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @buggmagnet)

Reviewed 1 of 5 files at r3, 1 of 3 files at r4, 1 of 5 files at r5, 1 of 2 files at r6, 3 of 5 files at r7.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mojganii)

ios/Shared/LaunchArguments.swift line 51 at r7 (raw file):

        let data = try encoder.encode(self)
        guard let result = String(bytes: data, encoding: .utf8) else {
            throw StringDecodingError(data: data)

That should be a StringEncodingError here

Reviewable status: all files reviewed, 1 unresolved discussion